src/db/car/forest/index/mod.rs (1)

195-220: Add rustdoc for the new public reader type and methods.

sZstdSkipFramesEncodedDataReader and its public API should be documented for maintainability and correct usage expectations.

As per coding guidelines: "Document all public functions and structs".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/db/car/forest/index/mod.rs` around lines 195 - 220, Add Rustdoc comments
for the public struct ZstdSkipFramesEncodedDataReader and its public methods
(new, inner, into_inner): document the purpose of the reader, the meaning of
skip_frame_header_offsets, the behavior of new (what it reads from the provided
ReadAt, how offsets are computed, and that it returns io::Result on failure),
what inner returns (borrowed reader) and what into_inner returns (consumes and
returns the inner reader). Keep the docs concise, include parameter/return
descriptions and any error/edge-case notes (e.g., empty input or partial
frames), and add a short usage example or note about thread-safety if
applicable.

src/db/car/forest/index/mod.rs (2)

245-265: ⚠️ Potential issue | 🟠 Major

read_at still performs linear skip-frame lookup per call.

Line [248] scans skip_frame_header_offsets linearly on every read. With many frames, this becomes a major lookup bottleneck; switch to O(log n) frame mapping (binary search on precomputed logical boundaries).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/db/car/forest/index/mod.rs` around lines 245 - 265, read_at currently
scans skip_frame_header_offsets linearly every call (see read_at,
skip_frame_header_offsets, ZSTD_SKIP_FRAME_LEN, reader.read_at), causing O(n)
lookup cost; replace that loop with a binary search over a precomputed vector of
logical frame boundaries (e.g., frame_logical_starts or
cumulative_adjusted_offsets computed once when building the structure) to find
the frame index and the adjusted_pos in O(log n), then compute max_read_len from
the next boundary and perform reader.read_at(adjusted_pos, &mut
buf[..max_read_len]) followed by recursive/iterative continuation for the
remainder; ensure correct u64/usize conversions and keep behavior identical to
existing adjustments.

---

477-485: ⚠️ Potential issue | 🟠 Major

Chunk-size constant does not match the stated 128MiB intent.

Line [479] uses 128 * 1024 (128KiB), which creates far more skip frames than intended and amplifies read-path overhead.

Proposed fix

-        const CHUNK_FRAME_DATA_MAX_BYTES: usize = 128 * 1024;
+        const CHUNK_FRAME_DATA_MAX_BYTES: usize = 128 * 1024 * 1024;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/db/car/forest/index/mod.rs` around lines 477 - 485, The
CHUNK_FRAME_DATA_MAX_BYTES constant in write_zstd_skip_frames_into is set to 128
* 1024 (128KiB) but the intent is 128MiB; update CHUNK_FRAME_DATA_MAX_BYTES to
128 * 1024 * 1024 (or an equivalent expression like 128 << 20) inside
write_zstd_skip_frames_into so the call to write_zstd_skip_frames_into_inner
uses the correct 128MiB chunk size, leaving the rest of the function (including
the call to write_zstd_skip_frames_into_inner and
u32::try_from(written_len).ok()) unchanged.

src/db/car/forest/index/mod.rs (1)

195-220: Add rustdoc for the new public reader API.

Line [195] introduces a public struct and Lines [201], [214], and [218] expose public methods without documentation. Please add concise docs for expected input format and malformed-data behavior.

As per coding guidelines "Document all public functions and structs".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/db/car/forest/index/mod.rs` around lines 195 - 220, Add Rustdoc comments
for the public struct ZstdSkipFramesEncodedDataReader and its public methods
new, inner, and into_inner: describe the expected input format (a sequence of
skip-frame headers followed by ZSTD_SKIP_FRAME_LEN + payload length, with
lengths read as little-endian u32 at offset+4), state that new parses the entire
reader to build skip_frame_header_offsets, and document malformed-data behavior
(what happens when read_u32_at fails — e.g., it returns an io::Result error from
new — and that inner/into_inner simply return the wrapped reader without
validation). Keep the docs concise, mention ownership/borrowing semantics for
inner vs into_inner, and reference ZSTD_SKIP_FRAME_LEN for clarity.

src/db/car/forest/index/mod.rs (1)

245-265: ⚠️ Potential issue | 🟠 Major

read_at remains linear per call and can bottleneck large multi-frame indices.

Line 248 iterates all skip-frame offsets, and Line 264 recurses, so repeated reads across many frames can degrade heavily. Please switch to indexed lookup (e.g., binary-search-based frame mapping) to keep lookup cost predictable at scale.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/db/car/forest/index/mod.rs` around lines 245 - 265, The read_at
implementation on read_at is doing a linear scan over skip_frame_header_offsets
and recursive reads, causing O(n) per call and worst-case O(n^2) across many
frames; replace the linear iteration/recursion with an indexed lookup (e.g.,
binary_search on skip_frame_header_offsets) to map pos→frame quickly, compute
adjusted_pos once using the found index and ZSTD_SKIP_FRAME_LEN, then perform a
bounded single reader.read_at call (reader.read_at) for the chunk within that
frame and loop/iterate (not recurse) to continue for remaining bytes—update
read_at to use binary_search (or equivalent) over skip_frame_header_offsets to
find next_frame_pos and avoid per-call linear scans and recursion.

src/db/car/forest.rs (1)

166-166: ⚠️ Potential issue | 🟠 Major

Guard index range computation with checked subtraction.

Line 166 still performs unchecked index_end_pos - index_start_pos. Corrupt footer data can make this underflow; return InvalidData instead of panicking/wrapping.

Proposed fix

-        Ok((header, index_start_pos, index_end_pos - index_start_pos))
+        let index_size_bytes = index_end_pos.checked_sub(index_start_pos).ok_or_else(|| {
+            invalid_data(format!(
+                "invalid footer offsets: index_start_pos({index_start_pos}) > index_end_pos({index_end_pos})"
+            ))
+        })?;
+        Ok((header, index_start_pos, index_size_bytes))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/db/car/forest.rs` at line 166, The subtraction index_end_pos -
index_start_pos can underflow on corrupt footer data; replace the unchecked
subtraction with a checked subtraction (e.g.,
index_end_pos.checked_sub(index_start_pos)) and if it returns None return an Err
with InvalidData (std::io::Error::new(ErrorKind::InvalidData, ...)) instead of
producing a wrapped/negative value; update the Ok((header, ...)) return to use
the checked difference.

src/db/car/forest/index/mod.rs (1)

195-220: Add rustdoc for the new public reader API.

ZstdSkipFramesEncodedDataReader and its public methods are exported without documentation. Please add concise docs for expected input format, invariants, and failure modes.

As per coding guidelines, Document all public functions and structs.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/db/car/forest/index/mod.rs` around lines 195 - 220, Add Rustdoc comments
for the public ZstdSkipFramesEncodedDataReader struct and its public methods
(new, inner, into_inner): describe the expected input format (sequence of
skip-frame headers followed by frames, how header length is read via read_u32_at
with LittleEndian and the use of ZSTD_SKIP_FRAME_LEN), list invariants
(skip_frame_header_offsets contains offsets discovered during construction,
reader must implement ReadAt, the reader is not validated beyond iterating until
read_u32_at fails), and document failure modes (new returns io::Error if
underlying read_u32_at fails unexpectedly, or stops normally when EOF/invalid
header occurs; callers should handle potential partial/invalid data), plus any
ownership/borrowing behavior (inner returns a reference; into_inner consumes and
returns the inner reader). Ensure doc comments are concise, placed above the
struct and each method, and follow crate doc-style guidelines.

src/db/car/forest.rs (1)

166-166: ⚠️ Potential issue | 🟠 Major

Guard footer-derived index size math with checked_sub.

Line 166 still does unchecked subtraction on on-disk values. Corrupt footer data can underflow and panic/wrap instead of returning InvalidData.

Proposed hardening

-        Ok((header, index_start_pos, index_end_pos - index_start_pos))
+        let index_size_bytes = index_end_pos
+            .checked_sub(index_start_pos)
+            .ok_or_else(|| invalid_data("invalid footer: index start is after index end"))?;
+        Ok((header, index_start_pos, index_size_bytes))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/db/car/forest.rs` at line 166, The subtraction index_end_pos -
index_start_pos is unchecked and can underflow on corrupt footer data; replace
it with a checked subtraction (use index_end_pos.checked_sub(index_start_pos))
and if it returns None return an Err(io::Error::new(ErrorKind::InvalidData,
...)) instead of panicking—update the return for Ok((header, index_start_pos,
...)) to use the validated size and ensure the error path yields InvalidData;
refer to the symbols header, index_start_pos, and index_end_pos to locate the
change.

src/db/car/forest.rs (1)

179-192: Add rustdoc for public APIs touched in this change.

index_size_bytes and into_dyn are public and currently undocumented.

As per coding guidelines, "Document all public functions and structs".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/db/car/forest.rs` around lines 179 - 192, Add concise rustdoc comments
for the public methods index_size_bytes and into_dyn (and the public ForestCar
type if not already documented): describe what value index_size_bytes returns
(units and meaning), and explain into_dyn’s behavior and return type (that it
converts the ForestCar into a dynamic boxed RandomAccessFileReader wrapped in
io::Result). Place the docs immediately above the fn signatures
(index_size_bytes, into_dyn) and ensure they follow Rust doc conventions (///)
and mention error conditions for into_dyn if any.